[kernel 726] fix web bot auth extension #110
Conversation
archandatta
force-pushed
the
archand/kernel-726/fix-web-bot-auth-extension
branch
from
de5debc to
1af7c34
Compare
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
archandatta
force-pushed
the
archand/kernel-726/fix-web-bot-auth-extension
branch
from
1af7c34 to
f9e1cee
Compare
rgarcia
left a comment
rgarcia
left a comment
There was a problem hiding this comment.
Good progress on the Chrome enterprise policy implementation. The core logic for separating policy vs non-policy extensions looks solid.
Main concern: the deleted TestWebBotAuthInstallation test should be updated rather than removed - the new flow has significant behavior changes that warrant e2e coverage.
A few minor nits on style/perf, and a question about whether the /update.xml root route is necessary.
|
|
||
| // Try to extract Chrome extension ID from update.xml | ||
| chromeExtensionID := extensionName | ||
| extractionErr := error(nil) |
There was a problem hiding this comment.
nit: var extractionErr error is more idiomatic
|
|
||
| // Serve update.xml at root for Chrome enterprise policy | ||
| // This serves the first update.xml found in any extension directory | ||
| r.Get("/update.xml", func(w http.ResponseWriter, r *http.Request) { |
There was a problem hiding this comment.
this serves the first update.xml found based on directory iteration order, which isn't deterministic. if there are multiple extensions with update.xml files, this could return the wrong one. is this route needed given /extensions/{name}/update.xml already works via the wildcard route above?
| // Filter out entries that start with the same extension ID | ||
| extensionIDPrefix := chromeExtensionID + ";" | ||
| policy.ExtensionInstallForcelist = slices.DeleteFunc(policy.ExtensionInstallForcelist, func(entry string) bool { | ||
| return len(entry) >= len(extensionIDPrefix) && entry[:len(extensionIDPrefix)] == extensionIDPrefix |
There was a problem hiding this comment.
nit: strings.HasPrefix(entry, extensionIDPrefix) would be cleaner here
| return "", fmt.Errorf("appid attribute is empty in update.xml") | ||
| } | ||
|
|
||
| // Validate extension ID format: Chrome extension IDs are 32 lowercase hex characters |
There was a problem hiding this comment.
nit: comment says "hex" but Chrome extension IDs use a-p (a base16 variant), not 0-9a-f. maybe "32 lowercase a-p characters" to match the regex
|
|
||
| // Validate extension ID format: Chrome extension IDs are 32 lowercase hex characters | ||
| // This prevents injection attacks via semicolons or other special characters | ||
| if !regexp.MustCompile(`^[a-p]{32}$`).MatchString(appID) { |
There was a problem hiding this comment.
nit: this compiles the regex on every call. could move to a package-level var extensionIDRegex = regexp.MustCompile(...)
| return targets, nil | ||
| } | ||
|
|
||
| func TestWebBotAuthInstallation(t *testing.T) { |
There was a problem hiding this comment.
this test was verifying the web-bot-auth policy installation flow. rather than deleting it, could we update it to test the new behavior? the new flow (requiring update.xml + .crx, ExtensionInstallForcelist, update_url instead of path) seems important enough to have e2e coverage.
Note
Implements Chrome enterprise policy–based extension installation and updates runtime flags handling.
ExtensionInstallForcelist, switchExtensionSettingsto useupdate_url, and addExtractExtensionIDFromUpdateXML;AddExtensionnow uses Chrome extension ID (fromupdate.xml) and maintains the forcelistupdate.xmland a.crx, and call updated policy API; only add--load-extensionflags for non-policy extensionsupdate.xml/.crxvia new HTTP routes (/extensions/*,/update.xml,/{filename}.crx) so Chrome can fetch them/chromium/flagsbind mount writableWritten by Cursor Bugbot for commit 058a6aa. This will update automatically on new commits. Configure here.